Add a high-level API for creating repodata #271
Conversation
|
Not sure why that test would be failing. edit: Interesting, it passes on Fedora 33, and fails on Fedora 34 Did something change with libxml? Filed #272 |
|
yea the failing tests are unrelated, in addition to the libxml2 issue you described there is also a zchunk regression. Regarding the PR it self, I personally would be fine with merging this. Some kind of a higher-level API that is in spirit closer to the createrepo_c binary seems nice to me. |
dralley
force-pushed
the
new-api
branch
2 times, most recently
from
ef94963
to
ccb8a6d
Compare
|
I really like the idea of a simpler high level API too. |
|
Great, I'll keep working on it then. |
dralley
changed the title
dralley
changed the title
dralley
force-pushed
the
new-api
branch
8 times, most recently
from
34ec1be
to
e70835b
Compare
|
I have some API design questions, see the TODO notes |
dralley
force-pushed
the
new-api
branch
4 times, most recently
from
1b56c81
to
babcaf3
Compare
src/python/createrepo_c/__init__.py
Outdated
| if preserve_existing_metadata: | ||
| x = 0 | ||
| while True: | ||
| new_repodata_path = f"{self.repodata_path}_{x}" |
There was a problem hiding this comment.
I think createrepo_c binary and RepositoryWriter should be as consistent as possible. How about using something closer to --retain-old-md and --keep-all-metadata instead of preserve_existing_metadata?
Or do you prefer this behavior?
There was a problem hiding this comment.
This is just what the example had been doing when I copied it over. I think there's a good argument to be made either way. It does seem a little silly to keep the metadata files around but throw away the repomd.xml that tied them together. If you keep the old repodata directories, it's fairly straightforwards to turn it back into a functional repo (assuming the packages are still there)
src/python/createrepo_c/__init__.py
Outdated
| sqlite_metadata_info.writer.close() | ||
| else: | ||
| record = RepomdRecord(record_name, str(metadata_info.path)) | ||
| record = record.compress_and_fill(self._metadata_checksum_type, BZ2_COMPRESSION) |
There was a problem hiding this comment.
While trying out the RepositoryWriter I noticed the created repositories are invalid. I think this section specifically exposes a bug in the library.
record.compress_and_fill(self._metadata_checksum_type, BZ2_COMPRESSION) returns a new record with filled in checksums while the old one (created on the line above - 603) is freed. However these two lines: https://github.com/rpm-software-management/createrepo_c/blob/master/src/repomd.c#L557-L558 make the new record use chunk from the old (freed) one. This leads to garbage values.
Could this be fixed as a part of this PR?
There was a problem hiding this comment.
@kontura It might take a while before this PR is ready depending on how much time I can dedicate to it. It should probably go in a separate PR. I can maybe take a shot at fixing it next week though.
There was a problem hiding this comment.
Ok, that is no problem.
I think we just need to change record->chunk to crecord->chunk for both of those lines. 🙂
dralley
force-pushed
the
new-api
branch
3 times, most recently
from
1176d3e
to
d516d7f
Compare
|
@kontura If I go forwards with this, are you ok with commits 2 and 3? |
|
Yes and maybe we could even merge |
|
This is ready to be looked at. I don't claim it's perfect but I tried to get decent test coverage. |
|
@kontura ^ |
|
Bump! |
|
@dralley sorry I am a bit swamped now, it might take a while. |
Abstracts away a lot of manual logic around setup and state tracking.
The existing API is frustratingly verbose and low-level. This will make it trivial to create repositories with only a few lines.
|
Updated |
|
Thank you! |
| self.working_metadata_files["filelists"].writer.add_pkg(pkg) | ||
| self.working_metadata_files["other"].writer.add_pkg(pkg) | ||
|
|
||
| def add_repomd_metadata(self, name, path, use_compression=True): |
There was a problem hiding this comment.
@kontura I didn't notice this until now (sorry), but add_repomd_metadata is a bit redundant. Should we fix that name? Or is it fine because we're adding metadata to "repomd"?
There was a problem hiding this comment.
So to change it to just add_metadata?
I think I would like that a little bit more but I am also fine with the current form.
If you want to change it I believe we still can but the sooner the better.
This is a mostly-functional prototype of a more convenient (Python) API for writing repos that I threw together.
There's no obligation to merge this, but, is it something you would be interested in merging? I figure that 90% of this code is code that any consumer of the API would need to write / copy anyway (and I started with just the example code).
If the answer is yes, I can finish it off, write documentation and tests, etc. Otherwise we can just close.